fix(agent): stop the grounding rule and compaction summaries from licensing fabricated answers - #2149
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refines the LLM prompts for evidence compaction and grounding rules to strictly forbid unsourced values (such as names, dates, statuses, and identifiers) by default, requiring the model to report missing data as gaps rather than inventing values. It also updates the compaction prompt to prioritize verbatim preservation of requested records and adds comprehensive unit tests. The review feedback highlights an inconsistency in auto.py where the list of forbidden value kinds is narrower than the centralized VALUE_KINDS in grounding.py, and suggests aligning both the prompt and its corresponding test assertion to ensure consistent routing behavior.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors and strengthens the grounding rules and LLM compaction prompts across the codebase to prevent data fabrication and hallucination (mitigating issue #1235). It introduces a centralized VALUE_KINDS definition to cover numbers, names, identifiers, dates, statuses, and table rows, and updates the grounding prompt to strictly forbid inventing these values by default, requiring gaps to be reported instead of using illustrative placeholders or caveats. The LLM compaction prompt is also updated to ensure verbatim preservation of requested records, forbid unearned completeness claims, and prioritize content when under a tight token budget. Corresponding updates are made to the Auto and DAG execution patterns, along with comprehensive test coverage validating these new behaviors. No review comments were provided, so there is no additional feedback.
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
execution.py:1245,1412 (outside this diff, see pointer on execution.py:1480) — [major] compaction trailer / dropped-tool notice still only names "figure", not the broader value-kind scope fixed elsewhere in this PR.
grounding.py:98-105 — [minor] template/sample exception scoped to "the answer" doesn't reach tool-argument content.
grounding.py:74 — [minor] tool_argument_rule keeps its own literal list instead of reusing VALUE_KINDS.
execution.py:1485 — [minor] "remaining work" phrasing risks being read as an implicit next-step instruction.
test_context.py:1252 — [minor] docstring miscites COMPACT_SUMMARY_MIN_TOKENS as basis for the 330-word input-prompt cap.
grounding.py:96 — [minor] relative clause reads as binding only to the last VALUE_KINDS list item.
grounding.py:99 — [minor] "a sample" trigger phrase is ambiguous between mockup content and a real example.
test_grounding.py:226 — [minor] ordering assertion checks two hardcoded substrings with no reorderable code path behind them.
grounding.py:17 / test_grounding.py:262 — [minor] denylist checks exact phrase "instructed default"; a synonymous rewrite evades it.
test_grounding.py:577 — [minor] coverage regression: no current test positively asserts the page-size/result-limit clause on the can_call_tools=True path.
test_dag.py:423 — [minor] assertion checks a phrase that never existed in dag.py's prompt; vacuously true.
test_context.py:1100 — [minor] _build_llm_compact_prompt_texts duplicates existing setup at ~1046-1055 instead of a shared helper.
Blocking: yes — recommended event: REQUEST_CHANGES
execution.py:1245,1412 — [new] compaction trailer/dropped-tool notice still only names "figure", missing the name/org/date/status types the incident actually fabricated
The rule used to require an up-front disclaimer for unsourced figures but otherwise let the model present them, and scoped that disclaimer duty to figures and numbers specifically. A model could satisfy it by inventing rows and appending a label copied near-verbatim from the rule text. Replace the disclosure duty with a default prohibition: unsourced values of any enumerated kind (numbers, person or organization names, identifiers, dates, statuses, table rows) must be omitted and reported as missing. The only exception is a current user request that explicitly asks for a template or sample, and even then the model must state the nature of the content before presenting it rather than appending a caveat afterward. The exemption for wording the model must compose itself (search queries, code, document text) is unconditional so all four call sites -- including the three forced-answer sites that previously lacked any prompt covering this -- carry it, while facts written literally inside such composed text remain subject to the sourcing rule. Also fix the module and function docstrings, which claimed disclosure was the instructed default; that stopped being true once fabrication without a source became a prohibition rather than a labelling obligation.
Both patterns follow their grounding_rule() call with a local sentence
that named the old wording ("unsupported specifics", "illustrative
placeholder"). The rule no longer uses that vocabulary, so those
sentences pointed at wording the model would never see.
Auto's routing remedy is reworded to name the value kinds the rule
forbids without changing its own job (route to react so a tool can
supply the value). DAG's assessment sentence is reworded to describe
leaving out a value rather than labelling an illustrative placeholder,
matching the rule's gap-reporting instruction.
The compaction summary prompt asked the summarizer to "name the next action needed" and told it the next call should continue "without redoing completed tool calls." Both fabrication-incident summaries put a do-not-call-tools instruction in that slot; the summarizer has no visibility into what tools the next call will actually have. Remove the next-action instruction entirely and state explicitly that the summary must not instruct the next call on tool use. Require fact-carrying values for the records the current request points at (names, identifiers, statuses, dates, counts) to be copied character for character rather than paraphrased or invented to complete a pattern, with credentials and unrelated personal information excluded even when they would otherwise qualify. Forbid claiming a dataset is complete unless the history shows every item was both returned and is still described in the summary -- the incident's dataset was fully returned across nine tool calls, but four of those calls' raw payloads had already been dropped by an earlier compaction. Give the smallest output budget an explicit priority order for what survives, and cap the prompt itself so it cannot silently outgrow the budget it exists to fit inside. The trailer appended after the summary is unchanged; it already carries a correctly-conditioned "re-read or re-query the source" instruction and is not part of this text.
…medy The auto pattern's routing-remedy sentence and its test assertion each hand-wrote a five-item value-kind list that had drifted from the grounding module's six-item VALUE_KINDS constant (missing "a status"). Both now interpolate VALUE_KINDS directly, so the routing remedy always matches the same value kinds the grounding rule itself forbids, with no second literal copy left to fall out of sync.
cad2a02 to
2829216
Compare
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Round 2: prompt-engineering fixes are correctly scoped but this remains pure prompt text with no code-level enforcement, and no test (here or in prior rounds) exercises the forced-final-answer x compaction crossing from #2146.
Minor
src/xagent/core/agent/context/execution.py:1467— The lengthened prefix in_dropped_tool_results_noticeshrinks the tool-name char budget (current_chars = len(prefix)) from ~746 to ~629 chars. Repro with 20 realistic MCP-style tool names (~30 chars) drops 3 extra names into "+N more" that used to fit.tests/core/agent/test_context.py:1455only trips the 20-name cap with short names, never this budget path. Reserve headroom in the budget constant or add a longer-name test.src/xagent/core/agent/pattern/auto/auto.py:1292— "that no source here supports" reads as attached only to the last item of theVALUE_KINDSdisjunction, not the whole list.grounding.py:99fixed this exact ambiguity with different dash placement; match that pattern here.src/xagent/core/agent/context/execution.py:1304— "or report the value as unavailable" is grammatically scoped only to the write-tool branch; the no-read-tool-available case has no explicit fallback in this sentence (covered only implicitly by the general rule ingrounding.py:58-61).src/xagent/core/agent/grounding.py:25—VALUE_KINDSis defined here buttool_argument_ruleandexecution.py:1517-1521's hand-written value-kind list don't reuse it, inviting future drift between lists (coverage itself is not currently broken).src/xagent/core/agent/grounding.py:89— TheVALUE_KINDSsplice reads as a closed enumeration at this andexecution.py:1299,1467. Two of the four sites already hedge ("any value", "or other value"); add "including but not limited to" at the closed-looking ones for consistency.src/xagent/core/agent/context/execution.py:1518— The credential-exclusion clause ("the exclusion wins") only resolves identifier-vs-credential collisions, not handle-vs-credential (e.g. a URL that's both an artifact handle and carries embedded auth). Wording hardening only — no in-scope handle in this system carries embedded credentials today.src/xagent/core/agent/context/execution.py:1534— "Separate completed work from remaining work" sits next to "write no instruction about tool use," which is where both observed fabrication summaries put a directive. Author agrees this is a real failure shape but left the order as-is; the zero-cost reorder is still worth doing.tests/core/agent/test_grounding.py:301—test_grounding_module_docstring_states_the_default_as_a_prohibitiondenylists specific phrases plus a positive pin, but a docstring rewrite that keeps both required phrases while reintroducing "disclose instead of omit" in different words would pass undetected.tests/core/agent/test_context.py:1111—_build_llm_compact_prompt_textsduplicates setup already attest_context.py:1046-1055. Extracting just the five shared setup lines into a small parametrized helper would remove the duplication without touching either test's return-value contract.
Simplification
- L159-212: delete: two near-identical banned-phrase-absence test functions (
test_grounding_rule_offers_no_reusable_disclaimer_phrasing,test_grounding_rule_does_not_license_labelled_fabrication) with the same loop shape over the same two rule variants. Merge into one@pytest.mark.parametrizetest over the combined phrase list; keep the one asymmetric "plausible-looking placeholder" check as a separate assertion.
net: -12 lines possible
Blocking: no — recommended event: APPROVE
The dropped-tool notice prefix spells out the shared value-kind list, and at 1024 characters that prefix crowded real tool names out of the very list of lost evidence the notice exists to give; 1152 holds a full page of names at the length an MCP server produces. Punctuate auto's routing remedy so "that no source here supports" qualifies the value the answer would need rather than the last kind in the list, and tell the compaction trailer's reader to report a value as unavailable when no tool can supply it at all. In the compaction prompt, the credential exclusion now covers a handle the request points at as well as an identifier, and the sentence forbidding instructions to the next call sits with the other rules about what the summary may say. Tests take the single-dropped-observation setup from one shared helper.
3 participants
Summary
This PR rewrites three prompt texts used by the agent: the grounding rule shared by every pattern, the compaction summary prompt, and the two sibling sentences that follow the grounding rule call in the auto and dag patterns. No execution logic changes; every diff is inside a string literal or a docstring. The goal is to close two gaps that let a model present unsourced or fabricated content to a user as if it were real: a labelling loophole in the grounding rule, and a next-action instruction in the compaction summary prompt that neither summarizer nor caller could actually honor.
Behavior changes
VALUE_KINDSconstant, so a value kind added to the rule reaches them automatically. The first one keeps the "statistic" and "quotation" items it already carried, which the shared list does not have. These two sentences predate this PR (they landed in5d53bef0, 2026-08-10) and were previously outside its diff; they are pulled in because they are the last prompt strings on this surface that stated a value-kind list of their own.Product decision disclosed
#1237 (proposal A of #1235) introduced a labelling clause for unsourced content and recorded "block outright vs allow with a label" as an open product decision. This PR resolves it in favor of blocking by default, with a single exception for a current user request that explicitly asks for a template or sample. Maintainers should weigh in on whether that is the right default.
Not in this PR
Verification
6f1f9ac2; head is28292169.VALUE_KINDSreference for a hand-written five-item list missing one entry ->test_grounding_rule_keeps_literal_facts_inside_composed_text_sourcedtest_compact_prompt_ranks_what_to_keep_when_the_budget_is_shorttest_compact_prompt_excludes_credentials_even_when_also_an_identifiertest_dag_completion_assessment_prompt_includes_grounding_ruletest_grounding_module_docstring_states_the_default_as_a_prohibitiontest_compact_prompt_does_not_grow_past_its_measured_ceilingtest_grounding_rule_scopes_the_answer_as_argument_exemptiontest_compact_prompt_excludes_credentials_and_unrelated_personal_datatest_compact_with_llm_summarizes_history_and_preserves_current_usertest_compact_with_llm_reports_dropped_tool_results_by_nametest_grounding_rule_exception_requires_an_explicit_current_requestandtest_grounding_rule_exception_reaches_content_bound_for_a_tool_argumenttest_grounding_rule_exception_trigger_qualifies_what_a_sample_meanstest_grounding_rule_keeps_literal_facts_inside_composed_text_sourcedtool_argument_rule->test_grounding_rule_covers_fact_carrying_tool_arguments(before this round the same deletion left every test green)Part of #2146